Skip to content

Conversation

@adalpari
Copy link
Contributor

@adalpari adalpari commented Nov 5, 2025

Description

This PR has been up for a while, waiting the RS layer to support attaching the logs to a HE support ticket.
a part og it has been done, but we are still waiting tof the response endpoint to accept log ids as well.
However, since having the PR open requieres some periodic checkings, I'm asking for a review knowing that the ticket anwswer part is missing. It doesn't hurt though, since the feature is behind an experimental feature flag.

The PR fixes how the logs were collected and shown. And allow the user to send them when opening a new ticket.

Testing instructions

  1. Log into a WP.COM account
  2. Enable MODERN_SUPPORT FF
  3. Navigate through the app
  4. Go to Me -> Support -> Logs
  • Verify you see the recent log entries
Screenshot 2025-12-05 at 13 22 12 Screenshot 2025-12-05 at 13 22 25
  1. Create a support conversation checking the "include logs" switch
  • Open Zendesk and verify the logs UUIDs are included in the ticket
  • Copy the logs UUID and verify it's available in our encrypted logs server
Screenshot 2025-12-05 at 13 24 53

@wpmobilebot
Copy link
Contributor

Project dependencies changes

list
! Upgraded Dependencies
rs.wordpress.api:android:trunk-8670f3dab1c722d2cd7d7477b3fe7ca1e916a25d, (changed from trunk-3d2dafdc1f8b058b4ed9101673fdf690671da73c)
rs.wordpress.api:kotlin:trunk-8670f3dab1c722d2cd7d7477b3fe7ca1e916a25d, (changed from trunk-3d2dafdc1f8b058b4ed9101673fdf690671da73c)
tree
 +--- project :libs:fluxc
-|    \--- rs.wordpress.api:android:trunk-3d2dafdc1f8b058b4ed9101673fdf690671da73c
-|         +--- com.squareup.okhttp3:okhttp:4.12.0 -> 5.3.1 (*)
-|         +--- com.squareup.okhttp3:okhttp-tls:4.12.0
-|         |    +--- com.squareup.okhttp3:okhttp:4.12.0 -> 5.3.1 (*)
-|         |    +--- com.squareup.okio:okio:3.6.0 -> 3.16.3 (*)
-|         |    \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.8.21 -> 1.9.24 (*)
-|         +--- net.java.dev.jna:jna:5.18.1
-|         +--- rs.wordpress.api:kotlin:trunk-3d2dafdc1f8b058b4ed9101673fdf690671da73c
-|         |    +--- com.squareup.okhttp3:okhttp:4.12.0 -> 5.3.1 (*)
-|         |    +--- com.squareup.okhttp3:okhttp-tls:4.12.0 (*)
-|         |    +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.10.2 (*)
-|         |    \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.2.21 (*)
-|         \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.2.21 (*)
+|    \--- rs.wordpress.api:android:trunk-8670f3dab1c722d2cd7d7477b3fe7ca1e916a25d
+|         +--- com.squareup.okhttp3:okhttp:4.12.0 -> 5.3.1 (*)
+|         +--- com.squareup.okhttp3:okhttp-tls:4.12.0
+|         |    +--- com.squareup.okhttp3:okhttp:4.12.0 -> 5.3.1 (*)
+|         |    +--- com.squareup.okio:okio:3.6.0 -> 3.16.3 (*)
+|         |    \--- org.jetbrains.kotlin:kotlin-stdlib-jdk8:1.8.21 -> 1.9.24 (*)
+|         +--- net.java.dev.jna:jna:5.18.1
+|         +--- rs.wordpress.api:kotlin:trunk-8670f3dab1c722d2cd7d7477b3fe7ca1e916a25d
+|         |    +--- com.squareup.okhttp3:okhttp:4.12.0 -> 5.3.1 (*)
+|         |    +--- com.squareup.okhttp3:okhttp-tls:4.12.0 (*)
+|         |    +--- org.jetbrains.kotlinx:kotlinx-coroutines-core:1.10.2 (*)
+|         |    \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.2.21 (*)
+|         \--- org.jetbrains.kotlin:kotlin-stdlib:2.1.21 -> 2.2.21 (*)
-\--- rs.wordpress.api:android:trunk-3d2dafdc1f8b058b4ed9101673fdf690671da73c (*)
+\--- rs.wordpress.api:android:trunk-8670f3dab1c722d2cd7d7477b3fe7ca1e916a25d (*)

…logs-sharing-with-HE

# Conflicts:
#	gradle/libs.versions.toml
@adalpari adalpari changed the title CMM-886 create logs sharing with he CMM-886 logs sharing fix and send to he ticket Dec 5, 2025
# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/support/he/ui/HENewTicketScreen.kt
#	WordPress/src/main/java/org/wordpress/android/support/he/ui/HESupportActivity.kt
#	WordPress/src/main/java/org/wordpress/android/support/he/ui/HESupportViewModel.kt
@adalpari
Copy link
Contributor Author

adalpari commented Dec 5, 2025

@claude

@claude
Copy link
Contributor

claude bot commented Dec 5, 2025

Claude finished @adalpari's task —— View job


Code Review for PR #22337

Overview

This PR refactors the logs collection/sharing system and adds support for attaching logs to HE support tickets. The main changes include:

  • Replaced LogDay model with LogFile model for better file handling
  • Improved log file display and parsing logic
  • Added encrypted log upload functionality when creating support tickets
  • Enhanced file sharing through FileProvider
  • Better separation of concerns between log viewing and log sharing

Strengths

  1. Well-structured refactoring: The transition from LogDay to LogFile model simplifies the architecture and makes the code more maintainable
  2. Comprehensive test coverage: Excellent unit test coverage for ViewModels and Repository with good edge case handling (attachment size validation, video caching, error scenarios)
  3. Good error handling: Most error paths are properly handled with appropriate logging via AppLogWrapper
  4. Memory-conscious design: Log truncation to 100 lines prevents memory issues with large log files
  5. Proper resource cleanup: Video cache cleanup is handled in both onDestroy() and onCleared() preventing leaks
  6. Security-minded: Uses encrypted log upload and FileProvider for secure file sharing
  7. Good separation: LogFile model only loads content on-demand, not eagerly

⚠️ Issues & Recommendations

I've left inline comments on specific areas. Here's a summary of the key concerns:

1. Security & Privacy 🔒

  • Log Upload Verification (HESupportViewModel:142-155): Silent failures in log upload could mislead users into thinking logs were included when they weren't
  • Temporary File Exposure (LogsViewModel:95-107): Cached log files persist without cleanup, potentially exposing sensitive data
  • Cache Directory Permissions (LogsViewModel:99-104): Ensure cache directory has appropriate permissions

2. Code Quality & Maintainability 🔧

  • Incomplete Implementation (HESupportViewModel:177-180): TODO comment indicates includeAppLogs parameter for replies isn't implemented yet - should this be removed until backend is ready?
  • Parameter Naming (HESupportRepository:101): encryptedLogIds could be clearer - consider uploadedLogUuids
  • Complex Validation Logic (HESupportViewModel:265-311): The attachment validation could benefit from extraction into clearer helpers with explicit null handling

3. Performance & Resource Management

  • File Reading (LogsViewModel:144-167): No file size check before reading - could attempt to read huge files
  • Silent Truncation (LogsViewModel:158): File truncation happens silently without user notification
  • Error Context Loss (LogsViewModel:164): Exception handling returns empty list, losing error context

4. Testing 🧪

Good:

  • Comprehensive test coverage for ViewModels
  • Edge cases well tested (file size limits, cache behavior, error scenarios)
  • Good use of mocking for dependencies

Could improve:

  • Add integration tests for log upload flow
  • Test file permission scenarios
  • Test cleanup timing for cached files

📋 Additional Observations

  1. Line Length: Code respects the 120-character limit specified in CLAUDE.md ✅
  2. Error Messages: Good use of specific error types (GENERAL, FORBIDDEN, OFFLINE)
  3. Nullability: Proper null safety throughout Kotlin code
  4. Coroutines: Good use of IO dispatcher for file operations
  5. State Management: Clean StateFlow usage for reactive UI updates

🎯 Recommendations Priority

High Priority:

  1. Fix security concern with temporary file cleanup (LogsViewModel)
  2. Address incomplete includeAppLogs implementation or document limitation
  3. Add file size validation before reading logs

Medium Priority:
4. Improve log upload error handling and user feedback
5. Refactor attachment validation for better clarity
6. Add file size check and better error messaging

Low Priority:
7. Rename parameters for clarity (encryptedLogIdsuploadedLogUuids)
8. Add cache cleanup timing/strategy


📝 Summary

This is a solid refactoring that improves the logs system architecture. The code quality is good, with comprehensive tests and proper error handling. The main concerns are around security (temporary file cleanup), incomplete functionality (reply logs), and some edge cases in file handling.

The PR is ready to merge after addressing the high-priority security and incomplete implementation concerns. The other recommendations are nice-to-haves that can be addressed in follow-up PRs.

Note: Since the feature is behind the MODERN_SUPPORT feature flag, the incomplete reply logs functionality is acceptable for now, but should be tracked and completed before general release.


Review completed

@adalpari adalpari marked this pull request as ready for review December 5, 2025 12:15
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 5, 2025

@codecov
Copy link

codecov bot commented Dec 5, 2025

Codecov Report

❌ Patch coverage is 44.91018% with 92 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.08%. Comparing base (9938dc1) to head (2c89119).
⚠️ Report is 2 commits behind head on trunk.

Files with missing lines Patch % Lines
...ordpress/android/support/logs/ui/LogsListScreen.kt 0.00% 39 Missing ⚠️
...rdpress/android/support/logs/ui/LogDetailScreen.kt 0.00% 23 Missing ⚠️
...dpress/android/support/he/ui/HESupportViewModel.kt 40.00% 13 Missing and 2 partials ⚠️
...wordpress/android/support/logs/ui/LogsViewModel.kt 83.33% 9 Missing and 3 partials ⚠️
...rdpress/android/support/he/ui/HENewTicketScreen.kt 0.00% 2 Missing ⚠️
...droid/support/he/repository/HESupportRepository.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #22337      +/-   ##
==========================================
+ Coverage   39.07%   39.08%   +0.01%     
==========================================
  Files        2207     2207              
  Lines      106795   106849      +54     
  Branches    15146    15157      +11     
==========================================
+ Hits        41732    41765      +33     
- Misses      61553    61570      +17     
- Partials     3510     3514       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants